Skip to content

Conversation

@tiran
Copy link
Contributor

@tiran tiran commented Jul 24, 2024

Fixes #196

commit 81a69bb

    Remove calls to logging.basicConfig on import
    
    Fixes #196
    
    instructlab.sdg calls logging.basicConfig on import.
    
    Libraries should not configure logging themselves. It's the job of the
    application or main script to configure logging and set up log
    handlers according to their needs. logging.basicConfig is a one-shot
    API. The second and any following call are a no-op, unless it is
    called with force=True.
    
    Remove setup_logging and just use logging.getLogger in the code.
    
    Co-authored-by: Christian Heimes <[email protected]>
    Signed-off-by: Mark McLoughlin <[email protected]>

commit 76a8624

    Use module level logger
    
    `generate_data` now uses a module-level logger like the rest of the
    code. The `logger` parameter is now optional.
    
    Signed-off-by: Christian Heimes <[email protected]>

@tiran
Copy link
Contributor Author

tiran commented Jul 24, 2024

@derekhiggins please take a look

@derekhiggins
Copy link
Contributor

hmm, we also have setup_logger in https://github.com/instructlab/sdg/blob/main/src/instructlab/sdg/logger_config.py
I wonder was the intent to converge on using that?

@tiran
Copy link
Contributor Author

tiran commented Jul 24, 2024

Is instructlab.sdg also designed to be used as a standalone application? Or is it only a library?

@tiran
Copy link
Contributor Author

tiran commented Jul 24, 2024

@derekhiggins I filed #196 for the logging problem.

@markmc
Copy link
Contributor

markmc commented Jul 24, 2024

Is instructlab.sdg also designed to be used as a standalone application? Or is it only a library?

Just a library IMO 👍

@markmc markmc added this to the 0.2.1 milestone Jul 24, 2024
@mergify mergify bot added the needs-rebase label Jul 24, 2024
@mergify
Copy link
Contributor

mergify bot commented Jul 24, 2024

This pull request has merge conflicts that must be resolved before it can be
merged. @tiran please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@markmc markmc modified the milestones: 0.2.1, 0.2.2 Jul 26, 2024
tiran and others added 2 commits July 26, 2024 23:41
`generate_data` now uses a module-level logger like the rest of the
code. The `logger` parameter is now optional.

Signed-off-by: Christian Heimes <[email protected]>
Fixes instructlab#196

instructlab.sdg calls logging.basicConfig on import.

Libraries should not configure logging themselves. It's the job of the
application or main script to configure logging and set up log
handlers according to their needs. logging.basicConfig is a one-shot
API. The second and any following call are a no-op, unless it is
called with force=True.

Remove setup_logging and just use logging.getLogger in the code.

Co-authored-by: Christian Heimes <[email protected]>
Signed-off-by: Mark McLoughlin <[email protected]>
@mergify mergify bot removed the needs-rebase label Jul 26, 2024
@markmc markmc changed the title Use module level logger Remove calls to logging.basicConfig on import Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Library calls logging.basicConfig on import

3 participants